Add Stalled condition and top level observedGeneration - #7664

Codecov Report

❌ Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.44%. Comparing base (e21600e) to head (70cb79c).

@@            Coverage Diff             @@
##             main    #7664      +/-   ##
==========================================
+ Coverage   82.34%   82.44%   +0.09%     
==========================================
  Files         130      131       +1     
  Lines       15879    15901      +22     
==========================================
+ Hits        13076    13109      +33     
+ Misses       2516     2506      -10     
+ Partials      287      286       -1     

I'm pretty confident about status reporting now being correctly interpreted, because this PR uses the actual kstatus library in its tests. However, I am not a Helm user and have a doubt.

I think it is common use case that user deploys HTTPProxies in a way that results in an invalid state being reported initially:

1. User creates HTTPProxies as part of their Helm chart, they reference TLS Secrets that are not part of the chart itself
2. kstatus will report a Failed, which is correct because requests cannot be served yet.
3. Some time after Helm install, the TLS Secrets are created.

Previously in step (2) Helm did not know how to interpret status conditions in HTTPProxies and was not aware of the failure. After the PR, what will Helm users experience with helm --wait and/or --atomic? The kstatus "protocol" as such is generic and working as expected, but could the Failed state cause unexpected deployment issues.

@deepy since you work with Helm, do you have any insights or opinions on this PR and my question: #7664 (comment). No worries if you are too busy.

I'll take a look tonight 👍

For people who use Helm through Flux the given scenario is likely not going to be an issue since depending on how you package your app you'll likely have the TLS secret created at the same time as the HelmRelease CR that'll install the helm chart, or one step earlier in the process

But for the scenario where that's not the case, or where you have other resources that don't have a status that Flux understands then it might get interesting, the immediate scenario that comes to mind is a chart with a Certificate from cert-manager

So while this is easy in Flux (even if it's a bit awkward), for Helm users I don't think there's really any way to solve it. At least until hip-0025 gets implemented, which solves exactly this scenario
I'll continue thinking on it and see if I can find any additional scenarios to try this weekend, but if not I think this is Helm working-as-intended and the best way to work with it would be splitting such a chart so it can be ordered as intended

Thank you @deepy!

Yes I was also thinking the exact cert-manager scenario. Imagine an umbrella chart with both contour and cert-manager, then run helm upgrade --rollback-on-failure "if set, Helm will rollback the upgrade to previous success release upon failure" (was --atomic). The referred Secrets might not exist, helm sees failed HTTPProxies and rolls back. If the Current success status of HTTPProxy becomes a pre-condition for successful helm deploy, and Failed status means rollback, then this feature just causes problems. Failed HTTPProxies are to be expected, and failure should not affect the deployment.

I have now tested this with helm v4.2.4. I created a chart with an HTTPProxy that has a missing secret reference:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: test-proxy
spec:
  virtualhost:
    fqdn: test.example.com
    tls:
      secretName: missing-tls-secret
  routes:
    - services:
        - name: echoserver
          port: 80

The Secret is not part of chart. It is created later, either by cert-manager or manually by an administrator. I believe this is perfectly valid use case.

The results are:

1. helm install --wait

   This succeeded, but only because --wait is a no-op here. It defaults to --wait=hookOnly which does not check resource statuses.

2. helm install --wait=watcher

   Error: INSTALLATION FAILED: resource HTTPProxy/default/test-proxy not ready. status: Failed, message: At least one error present, see Errors for details
   

   This uses kstatus to check the status of all included resources and detects that the HTTPProxy has a Stalled condition set to True, which means failure.

3. helm upgrade --rollback-on-failure

   level=WARN msg="upgrade failed" name=test-app error="resource HTTPProxy/default/test-proxy not ready. status: Failed, message: At least one error present, see Errors for details"
   Error: UPGRADE FAILED: release test-app failed, and has been rolled back due to rollback-on-failure being set: resource HTTPProxy/default/test-proxy not ready. status: Failed, message: At least one error present, see Errors for details
   

   This fails for the same reason as scenario 2 because the --rollback-on-failure flag implies --wait=watcher.

Additionally, we cannot change the semantics of the HTTPProxy status just to "please" Helm in this scenario. The status of HTTPProxy has always reflected its capability to handle HTTP requests, as it should. Even when read via the kstatus library, the semantics must remain the same.

If speculating that there was a Helm-specific way to signal status exclusively to Helm, then it might be possible to synthesize a new status meant only as a success criteria for the installation, rather than indicating overall resource status. This would still raise the question which errors are acceptable and should be ignored for deployment purposes, and the answer might not be the same for every user. That said, kstatus is clearly a generic status library, not Helm-specific.

So I'm starting to think it is not good idea to implement, at least not for Helm --wait purposes, which was the original motivator.

There's also positive scenarios where it does make sense to implement it for Helm's --wait purposes

Maybe a bit contrived of an example but: say you have 2 steps, the first one deploys the certificate + other supporting infrastructure and the second one deploys your app
You then create a new certificate that you intend to change to, but you make a mistake and now you've got one good and bad certificate
You then upgrade your application and use --rollback-on-failure
The application tries to use the bad certificate so the upgrade fails and is rolled back

There's also positive scenarios where it does make sense to implement it for Helm's --wait purposes

True. So, it depends entirely on the deployment design if errors are to be expected. With quick browsing, it seems that Flux allows more flexibility in defining what exact resources are checked for health, and what exact condition to check, while I cannot see anything in Helm for this. There it seems to be all-or-nothing.